Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use boost program_options in example #288

Merged
merged 8 commits into from
Sep 23, 2024
Merged

use boost program_options in example #288

merged 8 commits into from
Sep 23, 2024

Conversation

abe-winter
Copy link
Member

@abe-winter abe-winter commented Sep 19, 2024

What changed

  • Use boost::program_options to pass in host + credentials in one of the examples

Why

Incidental change while using this example. Not in love with this part of boost, p-ranav/argparse seems to be the modern equivalent, and cmake friendly? (But haven't tried it).

Not sure this is necessary / useful here, feel free to discard if not appropriate for sdk examples.

@abe-winter abe-winter requested a review from a team as a code owner September 19, 2024 19:26
@abe-winter abe-winter requested review from njooma and gloriacai01 and removed request for a team September 19, 2024 19:26
@lia-viam
Copy link
Contributor

Doing this had occurred to me too but I ended up deciding against it, kind of for the reasons you describe that people largely don't like using Boost program options, and the SDK examples are supposed to be usable as kind of cookie cutter copy-paste projects that someone would then build their own code on top of, in which case they probably would just use another argument-parsing library.

@acmorrow
Copy link
Member

There is prior art for using boost::program_options in the C++ SDK examples, so I'm pretty good with it. And I'd prefer that we used it rather than introducing a new dependency. It may not be a perfect library but it does just fine.

@abe-winter
Copy link
Member Author

I don't feel strongly about this and don't have context in this repo, one of you needs to make the decision unilaterally

@acmorrow
Copy link
Member

I think it is @lia-viam's call either way.

@lia-viam
Copy link
Contributor

Hahah let's leave this one aside then but thanks for the PR anyway!

@lia-viam lia-viam closed this Sep 20, 2024
@lia-viam lia-viam reopened this Sep 20, 2024
@lia-viam
Copy link
Contributor

Oh wait just saw @acmorrow 's comment saying we already do this elsewhere! I think we can merge this in that case

@lia-viam
Copy link
Contributor

@abe-winter or anyone else, feel free to merge this when the build passes, I just merged main since it was blocking

@abe-winter
Copy link
Member Author

can you approve? repo is locked to sdk team I think

@lia-viam lia-viam merged commit 9b2a576 into main Sep 23, 2024
3 checks passed
@lia-viam lia-viam deleted the dial-opts branch September 23, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants